feat(lammps): load installed backends dynamically#5707
Conversation
📝 WalkthroughWalkthroughThe PR refactors ChangesLAMMPS environment refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant lmp as deepmd/lmp.py (import time)
participant tfHelper as _get_tensorflow_library_paths
participant ptHelper as _get_pytorch_library_paths
participant env as os.environ
lmp->>tfHelper: get TF lib/preload paths
tfHelper-->>lmp: paths or empty (if TF missing)
lmp->>ptHelper: get PyTorch lib path
ptHelper-->>lmp: path or empty (if torch missing)
lmp->>lmp: compute CUDA paths (Linux)
lmp->>env: set preload_env via get_env
lmp->>env: set lib_env via get_env
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/test_lmp.py (1)
33-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the TF ≥ 2.12 branch.
Current coverage only exercises the
TF_VERSION < 2.12path (libpython preload). Add a complementary test withTF_VERSION="2.12.0"(or later) asserting the preload list stays empty, to guard the version-gated branch in_get_tensorflow_library_pathsagainst regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/tests/test_lmp.py` around lines 33 - 54, Add a complementary test for the TF_VERSION >= 2.12 branch in _get_tensorflow_library_paths, similar to test_tensorflow_library_paths_include_libpython_for_old_tensorflow. Mock deepmd.tf.env with TF_VERSION set to 2.12.0 or later and assert the returned TensorFlow library paths are still correct while the libpython preload list is empty, ensuring the version-gated logic does not regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@source/tests/test_lmp.py`:
- Around line 33-54: Add a complementary test for the TF_VERSION >= 2.12 branch
in _get_tensorflow_library_paths, similar to
test_tensorflow_library_paths_include_libpython_for_old_tensorflow. Mock
deepmd.tf.env with TF_VERSION set to 2.12.0 or later and assert the returned
TensorFlow library paths are still correct while the libpython preload list is
empty, ensuring the version-gated logic does not regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 256487fb-b134-4331-9a78-ff4abb1c8d12
📒 Files selected for processing (3)
deepmd/lmp.pydoc/install/easy-install.mdsource/tests/test_lmp.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5707 +/- ##
==========================================
- Coverage 81.22% 81.07% -0.16%
==========================================
Files 980 980
Lines 109352 109369 +17
Branches 4205 4208 +3
==========================================
- Hits 88821 88667 -154
- Misses 19011 19179 +168
- Partials 1520 1523 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| def _get_tensorflow_library_paths() -> tuple[list[str], list[str]]: | ||
| """Get TensorFlow library and preload paths when TensorFlow is installed.""" | ||
| try: | ||
| tf_env = import_module("deepmd.tf.env") | ||
| except ModuleNotFoundError as exc: | ||
| if exc.name == "tensorflow": | ||
| return [], [] | ||
| raise | ||
|
|
||
| tf_dir = tf_env.tf.sysconfig.get_lib() | ||
| preload_paths = [] | ||
| if Version(tf_env.TF_VERSION) < Version("2.12"): | ||
| find_libpython = import_module("find_libpython").find_libpython | ||
| libpython = find_libpython() | ||
| if libpython is not None: | ||
| preload_paths.append(libpython) | ||
| return [tf_dir, os.path.join(tf_dir, "python")], preload_paths | ||
|
|
||
|
|
||
| def _get_pytorch_library_paths() -> list[str]: | ||
| """Get PyTorch library paths when PyTorch is installed.""" | ||
| try: | ||
| torch = import_module("torch") | ||
| except ModuleNotFoundError as exc: | ||
| if exc.name == "torch": | ||
| return [] | ||
| raise | ||
| return [os.path.join(torch.__path__[0], "lib")] |
There was a problem hiding this comment.
Non-blocking (test coverage): source/tests/test_lmp.py is a welcome addition, but a few reachable branches of these new helpers aren't exercised. _get_tensorflow_library_paths is only tested with TF 2.11.0, so the TF >= 2.12 path (no libpython preload) and the find_libpython() is None case are untested; _get_pytorch_library_paths's success path (torch installed -> [torch/lib]) is never run because the _configure_lammps_environment tests monkeypatch the helper and the only direct torch test covers the missing-torch case; and the exc.name != "tensorflow"/"torch" re-raise branches (which intentionally propagate unrelated import errors) have no coverage. A couple of small cases would close these.
Summary
Tests
Fork CI runs:
Summary by CodeRabbit